Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix communication with DING #2220

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

sergio-costas
Copy link
Collaborator

The extension state naming has changed from gnome shell 45 to gnome shell 46, so the code to notify margins to DING wasn't being able to detect when an extension was active, and so it didn't prevent to put icons below the dock.

This patch fixes it.

@sergio-costas
Copy link
Collaborator Author

@3v1n0 Can you review this when you have some spare time, please?

The extension state naming has changed from gnome shell 45 to
gnome shell 46, so the code to notify margins to DING wasn't
being able to detect when an extension was active, and so it
didn't prevent to put icons below the dock.

This patch fixes it.
Copy link
Contributor

@smedir smedir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a. Why not just active? that would already means enabled.
b. What is happening right now? Gtk4-Ding is not having any issues at this moment on my desktop. Are there any errors being logged by dash-to-dock?

@sergio-costas
Copy link
Collaborator Author

@smedir Active is the new name in Gnome46 for what in Gnome45 was Enabled, so if we want to keep it working in both versions, both cases must be taken into account.

What happens is that if you have the intellihide option enabled, icons can be put under the dock, where they can be inaccessible unless you put a small window covering the dock in the opposite size, in order to hide it. This also happens with dash-to-panel and hide-top-bar.

@smedir
Copy link
Contributor

smedir commented May 15, 2024

if (usableArea?.uuid === IDENTIFIER_UUID) {

should fail automatically if the extension is not 'active', ie the enable method is not called, even when it is loaded, ie 'enabled', as the desktopIconItegrations class is supposed to be called and initialized when the extension is enabled, and nulled when disabled. Therefore usableArea should theoretically not exist at all.

So what I was wondering is where and how is this happening, and what, if any, error is being logged.

@sergio-costas
Copy link
Collaborator Author

sergio-costas commented May 16, 2024

@smedir The point is that you are supposing that an extensions is behaving correctly, which may not be the case, so I prefer to be cautious and check before if it is really enabled.

@smedir
Copy link
Contributor

smedir commented May 16, 2024

I essence, the code could be simplified to -

_sendMarginsToExtension(extension) {
        // check that the extension is an extension that has the logic to accept
        // working margins
        const usableArea = extension?.stateObj?.DesktopIconsUsableArea;

        if (usableArea?.uuid === IDENTIFIER_UUID)
            usableArea.setMarginsForExtension(this._myUUID, this._margins);
    }

This already makes sure the extension is 'enabled' and 'active'.

And I am not sure why the icons are coming under the dock, as this should be working properly. The error must be because of some other problem.

@sergio-costas
Copy link
Collaborator Author

@smedir The point is that up to Gnome45, it was used "ExtensionState.ENABLED" to specify that an extension was enabled; but in Gnome46 the Enum was changed, and that option was renamed to "ExtensionState.ACTIVE", so the original code compared the current state with "Undefined", thus always was FALSE, and that's why DING never received the margins data from Dash-to-Dock.

@sergio-costas
Copy link
Collaborator Author

sergio-costas commented May 16, 2024

In this PR I compare with both values to ensure that it works both in Gnome45 and Gnome46.

@smedir
Copy link
Contributor

smedir commented May 16, 2024

Looking at the original MR in Gnome shell here

45 has ENABLED vs DISABLED
46 has INACTIVE vs ACTIVE

The correct code would therefore be

if (!(extension?.state === ExtensionUtils.ExtensionState.ENABLED) ||
       (extension?.state === ExtensionUtils.ExtensionState.ACTIVE))

This also needs to be corrected on line 78 in the constructor as well, where we are connecting to the signal for state changes and then sending margins to extension.

Or if we wish to simplify the code and prevent any problems from state 'names' in future, just eliminate this check in line 78 constructor and sendMarginsToExtension, as const usableArea = extension?.stateObj?.DesktopIconsUsableArea; checks for this already.

@sergio-costas
Copy link
Collaborator Author

By DeMorgan, !(X or Y) = !X AND !Y ;-)

@sergio-costas
Copy link
Collaborator Author

But you are right about line 78, I have to fix that too.

@smedir
Copy link
Contributor

smedir commented May 16, 2024

In

By DeMorgan, !(X or Y) = !X AND !Y ;-)

😄 I would still suggest writing it !(X or Y) as it is more intuitive when reviewing the code, as that is what we are actually doing.

Or eliminating it altogether as I suggested as the usableArea? should take care of it.

@sergio-costas
Copy link
Collaborator Author

I think this is a simple fix.

@smedir
Copy link
Contributor

smedir commented May 16, 2024

Ah! I think that is much, much better, more intuitive to read and understand.

Strange placement for the _check...() method.

Would you also submit MR for dash-to-panel and other extensions using this class?

@sergio-costas
Copy link
Collaborator Author

I sent the path for dash-to-panel and hide-top-bar five minutes ago :-)

@vanvugt vanvugt merged commit 28e64a9 into micheleg:master Jun 10, 2024
1 check passed
@vanvugt
Copy link
Collaborator

vanvugt commented Jun 10, 2024

Ugh sorry for the 3 commit mess @3v1n0 and others. I approved this reviewing the diff alone, and apparently assumed it was a single commit like #2221.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants